lti changes #644
lti changes #644
Conversation
CodeSee Review Map:Review in an interactive map View more CodeSee Maps Legend |
jgwerner
left a comment
jgwerner
left a comment
There was a problem hiding this comment.
We identified new issues on unchanged lines of code. Navigate to the Amazon CodeGuru Reviewer console to view the recommendations to fix them.
jgwerner
left a comment
jgwerner
left a comment
There was a problem hiding this comment.
We identified new issues on unchanged lines of code. Navigate to the Amazon CodeGuru Reviewer console to view the recommendations to fix them.
| "ILLUMIDESK_NB_EXCHANGE_MNT_ROOT", "/illumidesk-nb-exchange" | ||
| ) | ||
| GRADER_PVC = os.environ.get("GRADER_PVC", "grader-setup-pvc") | ||
| GRADER_EXCHANGE_SHARED_PVC = os.environ.get( |
There was a problem hiding this comment.
Has the removal of GRADER_EXCHANGE_SHARED_PVC been tested? I was still having issues after I removed the deployment. That being said I was testing it without efs access points as we didn't need that in the Oregon cluster. I can try again with a new image and approve this afterward. Let me know if you have a new image, if not I'll fetch this PR and create the image and once tested, approve this.
There was a problem hiding this comment.
I have created a new image of this with pr-71 tag
| database_name: the database name | ||
| """ | ||
|
|
||
| def __init__(self, course_id: str, check_database_exists: bool = False): |
There was a problem hiding this comment.
So what I'm understanding here is that all courses will be in one database instead of a database-specific for each org_course. This assumes that the database exists by default.
There was a problem hiding this comment.
right, the DB will be common and will be maintained by the migrations in admin-dashboard code base
There was a problem hiding this comment.
we may need to add back the check_databse_exist boolean value in case the database does not exist
| return authentication | ||
|
|
||
|
|
||
| async def setup_user_hook_auth0( |
There was a problem hiding this comment.
Would the custom authenticator also need it's own Class that inherits from Oauthenticator in order to decode jwt claims?
There was a problem hiding this comment.
I think it should not be needed, this method is more list a post auth hook
jgwerner
left a comment
jgwerner
left a comment
There was a problem hiding this comment.
We identified new issues on unchanged lines of code. Navigate to the Amazon CodeGuru Reviewer console to view the recommendations to fix them.
|
We identified new issues on unchanged lines of code. Navigate to the Amazon CodeGuru Reviewer console to view the recommendations to fix them. |
3 participants
No description provided.